-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rnmobile/native toolbar component ui #11827
Conversation
Implement toolbar button style
I'll let @iamthomasbishop comment on this since I don't have the exact right answers, but things I notice so far:
It does look much nicer than before though 😁 |
@@ -3,18 +3,24 @@ | |||
*/ | |||
import { TouchableOpacity, Text, View } from 'react-native'; | |||
|
|||
import styles from './button-style.scss'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noting that it gives a false impression that this style file could be used by web because it uses .scss
extension without any indication that it can be used only by mobile. Please also note that we have a different convention for web. We use one file per folder and follow these guidelines when creating class names:
https://github.com/WordPress/gutenberg/blob/master/docs/reference/coding-guidelines.md#css
See also how files are named and included:
https://github.com/WordPress/gutenberg/blob/master/packages/components/src/style.scss
It feels like this is something you should standardize somehow sooner than later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want to continue to have it working the way it exists as of today, I would be fine if you rename those files to style.native.scss
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @gziolo , I have updated css files according your instructions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, looks good 👍
@@ -3,18 +3,25 @@ | |||
*/ | |||
import { TouchableOpacity, Text, View } from 'react-native'; | |||
|
|||
import styles from './style.native.scss'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we're not sharing styles with the web, I wonder if using StyleSheet.create
would look cleaner here to deal with conditional styling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does make sense. Fixed.
@@ -42,7 +43,7 @@ class IconButton extends Component { | |||
|
|||
let element = ( | |||
<Button aria-label={ label } { ...additionalProps } className={ classes }> | |||
{ isString( icon ) ? <Dashicon icon={ icon } /> : icon } | |||
{ isString( icon ) ? <Dashicon icon={ icon } ariaPressed={ariaPressed}/> : icon } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aria-pressed
is specific to the web. Instead of trying to hijack that, I'd prefer if we rename it to something more neutral (pressed
?), then make sure each platform does what it needs to render correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have that on a mind, but it seems that we are using aria-pressed in button/index.native.js so I wanted to be consistent with other parts of the code.
height="100%" | ||
width="100%" | ||
{ ...safeProps } | ||
/> | ||
); | ||
}; | ||
|
||
export const Path = ( props ) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why we need this wrapper, can you explain more?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! This was a valid solution at some point, but after a few iterations, it lost purpose!
Fixed.
Remove scss file and use proper StyleSheet to make code cleaner
Hey @iamthomasbishop Definitely looking closer!
It would be good to have it today as we want to merge this PR.
Nope. It's not easy at this point to do that. Updated design : |
This is looking great. Some minor details:
|
A little update on the default/un-selected button icon colors, as discussed in dm w/ @marecar3: I did a little digging to figure out why our icons looked too dim compared to Aztec, and I realized that we were using a custom shade for the toolbar in Aztec, which is slightly darker than |
# Conflicts: # packages/components/src/icon-button/index.js
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking great 👏
@@ -896,7 +898,7 @@ export default class Dashicon extends Component { | |||
return null; | |||
} | |||
|
|||
const iconClass = [ 'dashicon', 'dashicons-' + icon, className ].filter( Boolean ).join( ' ' ); | |||
const iconClass = IconClass( this.props ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a component or or a function, if it's a function it should be lower-case.
Also, we shouldn't touch this file directly, it's synced from the Dashicon repository I think, which means these changes can get removed inadvertently.
@@ -42,7 +43,7 @@ class IconButton extends Component { | |||
|
|||
let element = ( | |||
<Button aria-label={ label } { ...additionalProps } className={ classes }> | |||
{ isString( icon ) ? <Dashicon icon={ icon } /> : icon } | |||
{ isString( icon ) ? <Dashicon icon={ icon } ariaPressed={ ariaPressed } /> : icon } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I understand this change? why do we need aria-pressed here since we're applying it to the parent button?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The underlying issue is that we need to find a way to set the color of a Dashicon
. Since the web has CSS it can use class names, inheritance, and fill: currentColor
to get to that. But we don't have that on native.
Ideally we'd be able to set color
or style
on a Dashicon, I took an alternate approach in #12403, but that was easier since I was using Dashicon directly. For this we'd have to make everything in the hierarchy forward the style down to Dashicon. As mentioned on that PR, you can set extraProps
in IconButton
, but those will go to the Button
, not the icon.
We just found something that worked to keep us moving although we were aware it wasn't great, so I'm happy to discuss a better solution 😁
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm concerned about these small tweaks having an impact on the web code base, can't we create an icon-button
native version instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The underlying issue is that we need to find a way to set the color of a
Dashicon
. Since the web has CSS it can use class names, inheritance, andfill: currentColor
to get to that. But we don't have that on native.
@koke Can you point me to where this occurs? As best I can follow, the main impact of the ariaPressed
prop is in changing the class behavior of Dashicon
to apply dashicon-active
in the native context, but I see no reference to dashicon-active
anywhere in Gutenberg or Gutenberg Mobile aside from where we apply it.
I'd tend to agree with @youknowriad here. "Pressed" does not resound to me as a characteristic of an icon, and thus it does not make sense as a prop. It does, however, seem natural to describe the state of an IconButton
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is what's setting the style https://github.com/WordPress/gutenberg/blob/master/packages/components/src/primitives/svg/style.native.scss#L6
I agree that this isn't ideal. It seems like the introduction of iconProps
in #13977 could be a good solution to pass custom styles from IconButton
to Dashicon
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know that #13977 is an improvement here, as I expect you'd continue to treat it as a pressed icon, which is the root of the issue ("pressed" is not a sensible state of an icon).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, that's right.
It looks like the right logic here would be that IconButton
set different style props on Dashicon
depending on isActive
. This seems to be what the web is doing, although via CSS.
However this is overriden by CSS in the ToolbarButton
component.
We keep bumping into this on native: the web can rely on nested CSS definition to pass style down to lower level components, but we can't do that unless we pass them explicitly. I still think ToolbarButton
should be somehow deciding what the style for Dashicon
is when normal/pressed, which is why now having iconProps
in IconButton
seems like a way out.
@etoledom can you handle this (removing ariaPressed from Dashicon) as part of #13977?
Description
This PR resolves part of the Toolbar issue. It implements Toolbar and Toolbar Button style by the specs.
What's done :
Implement Toolbar Style
To test: